Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIPChanges to yaml files, deployment bicep script, and readme file. #247

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

francisnazareth
Copy link
Contributor

@francisnazareth francisnazareth commented Jan 31, 2025

azuredeploy.bicep

  • Removing use of service principal (client ID and client secret) for AKS cluster creation. (managed identity is preferred)
  • Removing Kubernetes version parameter (as default version is used)
  • Removing storage account creation (as this storage account is not used by any of the microservices).
  • Adding resource group contributor role assignment to the managed identity
  • Removing Log analytics workspace creation (as a log analytics workspace is already created by the fabrikam workload bicep script).
  • Changing the script to take in the existing log analytics workspace's resource ID as a parameter and create the AKS cluster using the log analytics workspace resource ID.

charts/package/template/package-deploy.yaml

The package microservice needs App insights connection string. Hence adding this connection string as a secret in the deployment yaml file.

charts/package/templates/package-secret-provider.yaml
adding a new key and value for application insights connection string (this will be used in the deployment yaml file)

Readme file:
Changes for readability, document structure, and ease of deployment (as the previous documentation was too complex).

@ckittel

@francisnazareth francisnazareth changed the title [WIPChanges to yaml files, deployment bicep script, and [WIPChanges to yaml files, deployment bicep script, and readme file. Jan 31, 2025
Copy link

@johndowns johndowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @francisnazareth - a few requests here, some of them just for stylistic/consistency reasons, but also a few more important items.

```

## Deployment
:book: This pre-flight Bicep template creates two resource groups. Additionally five User Identites are provisioned that will be later associated to every containerized microservice. This is because they will need Azure RBAC roles over the Azure KeyVault to read secrets in runtime. The resources will be created on the resouce group location and each resource group will contain the region as part of their names

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:book: This pre-flight Bicep template creates two resource groups. Additionally five User Identites are provisioned that will be later associated to every containerized microservice. This is because they will need Azure RBAC roles over the Azure KeyVault to read secrets in runtime. The resources will be created on the resouce group location and each resource group will contain the region as part of their names
:book: This pre-flight Bicep file creates two resource groups. Additionally five User identities are provisioned that will be later associated to every containerized microservice. This is because they will need Azure RBAC roles over the Azure Key Vault to read secrets at runtime. The resources will be created in the resource group's location, and each resource group will contain the region name as part of its name.


export PREREQS_DEPLOYMENT_NAME=workload-stamp-prereqs-main
### Assign ACR variables

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Assign ACR variables
### Assign Azure Container Registry variables

until az ad sp show --id $WORKFLOW_PRINCIPAL_ID &> /dev/null ; do echo "Waiting for Microsoft Entra ID propagation" && sleep 5; done
until az ad sp show --id $PACKAGE_ID_PRINCIPAL_ID &> /dev/null ; do echo "Waiting for Microsoft Entra ID propagation" && sleep 5; done
until az ad sp show --id $INGESTION_ID_PRINCIPAL_ID &> /dev/null ; do echo "Waiting for Microsoft Entra ID propagation" && sleep 5; done
1. Build and push the Delivery service container image to ACR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Build and push the Delivery service container image to ACR.
1. Build and push the Delivery service container image to the container registry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style nit, but could you please indent all of the code fences by four spaces, so they are indented underneath the corresponding step?

# Get outputs from workload deploy
export ACR_NAME=$(az deployment group show -g rg-shipping-dronedelivery-${LOCATION} -n workload-stamp --query properties.outputs.acrName.value -o tsv)
export ACR_SERVER=$(az acr show -n $ACR_NAME --query loginServer -o tsv)
2. Build and push the Ingestion service container image to ACR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
2. Build and push the Ingestion service container image to ACR.
2. Build and push the Ingestion service container image to the container registry.

```

Deploy the managed cluster and all related resources (This step takes about 15 minutes)
3. Build and push the Workflow service container image to ACR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
3. Build and push the Workflow service container image to ACR.
3. Build and push the Workflow service container image to the container registry.

## Deploy the managed cluster and related resources

```bash

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

export LOG_ANALYTICS_WORKSPACE_ID=$(az deployment group show -g rg-shipping-dronedelivery-${LOCATION} -n workload-stamp --query properties.outputs.laWorkspace.value -o tsv)

az deployment group create -g rg-shipping-dronedelivery-${LOCATION} --name managed-cluster-deployment --template-file azuredeploy.bicep \
--parameters deliveryIdName=uid-delivery \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency could you please indent this line?

```bash
export AKS_OIDC_ISSUER="$(az aks show -n $CLUSTER_NAME -g rg-shipping-dronedelivery-${LOCATION} --query "oidcIssuerProfile.issuerUrl" -otsv)"
```

## Deploy the Delivery service

Extract resource details from deployment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove this intruction, or update it to reflect what's actually happening? (There's a lot more than extracting resource details!)

Please do the same on each of the services below.

@@ -254,7 +241,7 @@ helm install delivery-v0.1.0-dev delivery-v0.1.0.tgz \
--set identity.tenantId=$TENANT_ID \
--set keyVaultName=$DELIVERY_KEYVAULT_NAME \
--set ingress.tls=true \
--set ingress.class=nginx \
--set ingress.class=webapprouting.kubernetes.azure.com \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I ran these steps on macOS (v15.3) using the default zsh shell, and the glob patterns caused an issue. I had to change these lines:

     --set ingress.hosts[0].name="$EXTERNAL_INGEST_FQDN" \
     --set ingress.hosts[0].serviceName=delivery \
     --set ingress.hosts[0].tls=true \
     --set ingress.hosts[0].tlsSecretName=delivery-ingress-tls \

to:

     --set ingress.hosts\[0\].name="$EXTERNAL_INGEST_FQDN" \
     --set ingress.hosts\[0\].serviceName=delivery \
     --set ingress.hosts\[0\].tls=true \
     --set ingress.hosts\[0\].tlsSecretName=delivery-ingress-tls \

```
kubectl get pods -n backend-dev --watch
```

## Validate the application is running

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first ran the curl request to the deliveryrequests API, I got a 503 response from nginx. I had verified the pods were running.

After a few minutes I was able to successfully reach it, so the problem was only temporary - but is there anything else to check for before running the validation steps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants